Skip to content

complete "yarn dev" to watch every last thing, including transitive less and pug imports#7556

Open
hatton wants to merge 9 commits intomasterfrom
bl-15641_more-vite-dev
Open

complete "yarn dev" to watch every last thing, including transitive less and pug imports#7556
hatton wants to merge 9 commits intomasterfrom
bl-15641_more-vite-dev

Conversation

@hatton
Copy link
Member

@hatton hatton commented Dec 30, 2025

This change is Reviewable


Open with Devin

Copilot AI review requested due to automatic review settings December 30, 2025 23:08

This comment was marked as resolved.

@hatton hatton force-pushed the bl-15641_more-vite-dev branch from af66281 to fefa970 Compare December 30, 2025 23:28
devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

Copy link

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 new potential issue.

View 16 additional findings in Devin Review.

Open in Devin Review

Comment on lines +470 to +477
this.pendingBuilds.set(
entryId,
next.finally(() => {
if (this.pendingBuilds.get(entryId) === next) {
this.pendingBuilds.delete(entryId);
}
}),
);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 queueBuild cleanup condition always evaluates to false because it compares next against next.finally(…)

The pendingBuilds map is never cleaned up because the finally callback compares the wrong promise reference. pendingBuilds.set(entryId, next.finally(cb)) stores the new promise returned by .finally(), but inside the callback, the check is this.pendingBuilds.get(entryId) === next — comparing the stored finally-wrapped promise against the original next. Since .finally() always returns a new promise object, this comparison is always false, so pendingBuilds.delete(entryId) never executes.

Root cause and impact

The pattern at line 470-477 is:

this.pendingBuilds.set(
    entryId,
    next.finally(() => {
        if (this.pendingBuilds.get(entryId) === next) { // always false
            this.pendingBuilds.delete(entryId);
        }
    }),
);

next.finally(cb) returns a different promise object (call it wrapped). The map stores wrapped, but the callback checks get(entryId) === next. Since wrapped !== next, the delete is never reached.

The fix is to capture the wrapped promise and compare against it:

const wrapped = next.finally(() => {
    if (this.pendingBuilds.get(entryId) === wrapped) {
        this.pendingBuilds.delete(entryId);
    }
});
this.pendingBuilds.set(entryId, wrapped);

Impact: Settled promises accumulate in pendingBuilds. Because each entry ID only ever has one value in the map, the leak is bounded by the number of LESS files (typically small), and build sequencing is unaffected since chaining onto a settled promise works correctly. The main consequence is that the intended cleanup is silently broken.

Suggested change
this.pendingBuilds.set(
entryId,
next.finally(() => {
if (this.pendingBuilds.get(entryId) === next) {
this.pendingBuilds.delete(entryId);
}
}),
);
const wrapped = next.finally(() => {
if (this.pendingBuilds.get(entryId) === wrapped) {
this.pendingBuilds.delete(entryId);
}
});
this.pendingBuilds.set(entryId, wrapped);
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

Comments